Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress TimeZone info on TimeData questions #942

Merged
merged 2 commits into from
Oct 28, 2020

Conversation

ctsims
Copy link
Member

@ctsims ctsims commented Oct 28, 2020

A forum user identified that they were having "off by an hour" questions which end up blocking web apps from working correctly, because the server doesn't send back the same value for questions as the browser provided.

This is because the TimeData construct has been improperly carrying forward TZ info internally with the time value, despite it being a "WallClock" representation. Since the internal calendar date for the TimeDate Date is the Epoch, if the TimeZone offset in the current locale differs from the offset on the Epoch, dates would drift. Kevin's recent change to improve Timezone accuracy on formplayer had the unintended side effect of instructing the browser about the current Locale instead of the current offset, triggering the behavior there.

This change simply strips the TZ info when the question converts the internal value back out to a serialized representation, since the TZ (like the Date itself) isn't actually a part of the TimeData object.

Formplayer commit here: dimagi/formplayer#778

Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! This would change for mobile as well right ? Also wondering if time values can be used in any calculations that might break because of the format change. If yes, may be it's worth announcing this change on commcare forums.

@knguyen142
Copy link
Contributor

Interesting, would this fix this issue https://dimagi-dev.atlassian.net/browse/SAAS-11477

@ctsims
Copy link
Member Author

ctsims commented Oct 28, 2020

@knguyen142 Oh, almost certainly, I forgot this was something that was being actively used there.

I'll wrap up the formplayer side of this now so it's ready to go. One silly thing is that this will really only be important for 2 more days, since we're about to flip back off of DST, but no reason to not get it resolved ASAP.

@knguyen142
Copy link
Contributor

Yes it'd be nice to see if this fixes it so we can prepare for next year. Thanks @ctsims and my apologies for not realizing that this would affect the time question.

@ctsims
Copy link
Member Author

ctsims commented Oct 28, 2020

@knguyen142 It's not your fault, the regression tests didn't catch the existing case since we don't have a strong way to test regressions currently in drifting locales . The original code shouldn't have been TZ dependent and was an existing bug that it was that just wasn't surfaced.

This PR and dimagi/formplayer#778 should be good to go for rollout as soon as tests pass there.

@ctsims
Copy link
Member Author

ctsims commented Oct 28, 2020

nice! This would change for mobile as well right ? Also wondering if time values can be used in any calculations that might break because of the format change. If yes, may be it's worth announcing this change on commcare forums.

@shubham1g5 I will double check this. I think the tz formatting might have only been showing up on the web because of the explicitly set TZ provider, but I could be wrong (or it could only affect half of the problem but not the other). You are right that if the field output changes we should let people know.

@ctsims ctsims linked an issue Oct 28, 2020 that may be closed by this pull request
@ctsims ctsims merged commit a036bf5 into formplayer Oct 28, 2020
@ctsims ctsims deleted the ctsims/time_question_tz_suppress branch October 28, 2020 20:37
@orangejenny
Copy link
Contributor

I will double check this.

The ticket Kevin linked does show that forms submitted via mobile are including time zone info when saving a time question to a case property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defaulting TimeData to "now" has offset issues
4 participants